fix: refuse userinfo in an object-store endpoint (#995) by OffgridwithJD · Pull Request #1064 · commandprompt/pgcolumnar

OffgridwithJD · 2026-09-14T15:25:15Z

Closes #995. #997 closed the caller's half — s3://u:p@bucket/key. This is the operator's half: userinfo in the endpoint, which the bucket guard cannot see because it is not in the URL at all.

Two shapes, and only one was ever caught

Measured on the authority parse before writing anything:

http://u:p@host:30829   -> host "u",          port 0       refused, wrong reason
http://user@host:30829  -> host "user@host",  port 30829   port VALID
http://host:30829       -> host "host",       port 30829   (clean control)

The first has a colon inside the userinfo, so the authority split lands there and the port becomes atoi("p@host:30829") = 0. The second has no such colon: the real port survives, the @ rides along in the host, and the invalid-port refusal never fires.

The issue measured the first shape and concluded "refused as invalid host or port". That is true of that shape rather than of the code.

The second shape is why this is a guard and not a message change

Its refusal came from the allow-list, naming the host it could not match, and the hint then told the operator:

ALTER SYSTEM SET pgcolumnar.objstore_allowed_endpoints = 'user@host'

A diagnostic that invites widening a security boundary to accommodate a parse bug is worse than a wrong error code. Following it moves the failure from the allow-list to a DNS miss — the operator has loosened their endpoint allow-list and still cannot read the object.

Placement: before the scheme and region demands

Not at the authority parse eighty lines later. Placed there the guard is unreachable whenever no region is configured, so an operator with a userinfo endpoint and no AWS_REGION is told about the region instead.

Measured — every endpoint arm returned requires a region option until it moved. That is the unreachable-guard trap the issue itself names, and the same reasoning the bucket guard carries for sitting before the endpoint is resolved at all.

The message names the endpoint rather than the URL, because that is the string carrying the userinfo; userinfo in "s3://bucket/key" would name a URL that has none.

Removal proof, on both harnesses

Each run asserts the .so was rebuilt and installed (mtime moved) before its verdict is read, and the source is restored to md5 0de468b1.

harness	control	guard removed	restored
`objstore_userinfo.sh`	26 passed	5 failed	26 passed
`test_objstore_endpoint_userinfo.py`	5 passed	3 failed	5 passed

Two traps walked into while measuring, both already in the record

PGC_SKIP_BUILD=0 SKIPS the build. lib.sh tests [ -z "${PGC_SKIP_BUILD:-}" ], so any non-empty value means skip — including 0, which reads as "do not skip". The first run measured a .so from the previous day and reported five failures against code that did not contain the guard. The suite printed PGC_SKIP_BUILD=1: not building AND NOT INSTALLING on line 5 of its own output and I did not read it; the timestamps did.

git checkout -- FILE reverted the uncommitted fix. The removal proof's restore took the file back to HEAD, which had no guard, so the final control failed identically to the mutant. Caught because the restore's md5 assertion did not fire. Fixed by committing before mutating.

Tests

Arms in test/objstore_userinfo.sh beside the existing ones, with a clean-endpoint control in the same run — without it these cannot tell a userinfo refusal from a foreign server that reaches nothing, which is exactly how the issue's first probe wasted a run.

And independently in test/pytest/test_objstore_endpoint_userinfo.py. That file is not a port and not a pair — objstore_endpoint_userinfo.sh does not exist. It asserts the same properties through the python harness, and nothing in it sources, invokes or reads anything under test/*.sh. It carries two arms the bash side does not: that the guard fires with no region configured (the placement claim, stated as a test rather than a comment), and that an @ in the object key is untouched — without which the arms would also pass on a guard that refused every @ anywhere.

objstore_userinfo.sh  26 checks, 0 failed   (pg16a assert build)
guard leg            340 passed, 867 checks, 0 fail
cluster leg          325 passed, 898 checks, 0 fail

cluster_tests 320 → 325 by collection.

One thing for @jdatcmd

The issue said "#706's author should say whether the endpoint message moves." I did not move the existing invalid host or port message — the guard now fires before that path is reached for any @-carrying endpoint, so that message is unchanged and still covers the genuine invalid-port cases. If you would rather the endpoint message read userinfo in "%s" to match the other three schemes exactly, rather than naming the endpoint, say so and I will change it.

🤖 Generated with Claude Code

https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

commandprompt#997 closed `s3://u:p@bucket/key` -- userinfo in the URL the CALLER writes. The endpoint the OPERATOR configures was still unguarded, and the bucket guard cannot see it, because it is not in the URL at all. TWO SHAPES, AND ONLY ONE WAS EVER CAUGHT. Measured on the authority parse before writing anything: http://u:p@host:30829 -> host "u", port 0 refused, wrong reason http://user@host:30829 -> host "user@host", port 30829 port VALID http://host:30829 -> host "host", port 30829 (clean control) The first has a colon INSIDE the userinfo, so the authority split lands there and the port becomes atoi("p@host:30829") = 0. The second has no such colon: the real port survives, the '@' rides along in the host, and the invalid-port refusal never fires at all. commandprompt#995 measured the first and concluded "refused as invalid host or port" -- true of that shape rather than of the code. THE SECOND SHAPE IS WHY THIS IS A GUARD AND NOT A MESSAGE CHANGE. Its refusal came from the allow-list, naming the host it could not match, and the hint then told the operator: ALTER SYSTEM SET pgcolumnar.objstore_allowed_endpoints = 'user@host' A diagnostic that invites widening a security boundary to accommodate a parse bug is worse than a wrong error code. Following it moves the failure from the allow-list to a DNS miss. PLACED BEFORE THE SCHEME AND REGION DEMANDS, not at the authority parse eighty lines later. Placed there the guard is unreachable whenever no region is configured, and an operator with a userinfo endpoint and no AWS_REGION is told about the region. MEASURED: every endpoint arm returned "requires a region option" until it moved -- the unreachable guard trap commandprompt#995 itself names. The message names the ENDPOINT rather than the URL, because that is the string carrying the userinfo. Removal proof, on both harnesses, each run asserting the .so was REBUILT AND INSTALLED (mtime moved) before its verdict is read: bash CONTROL 26 passed guard removed 5 failed restored 26 passed pytest CONTROL 5 passed guard removed 3 failed restored 5 passed Source restored to md5 0de468b1 after each. TWO TRAPS WALKED INTO WHILE MEASURING, both already in the record: `PGC_SKIP_BUILD=0` SKIPS THE BUILD. lib.sh tests `[ -z "${PGC_SKIP_BUILD:-}" ]`, so any non-empty value means skip -- including `0`, which reads as "do not skip". The first run measured a .so from the previous day and reported five failures against code that did not contain the guard. The suite said so on line 5 of its own output and I did not read it; the timestamps did. `git checkout -- FILE` REVERTED THE UNCOMMITTED FIX. The removal proof's restore took the file back to HEAD, which did not have the guard, so the final control failed identically to the mutant. Caught because the restore's md5 assertion did not fire. Fixed by committing before mutating. Arms in test/objstore_userinfo.sh beside the existing ones, with a clean-endpoint control in the same run, and independently in test/pytest/test_objstore_endpoint_userinfo.py -- which is NOT a port and not a pair, since `objstore_endpoint_userinfo.sh` does not exist. objstore_userinfo.sh 26 checks, 0 failed (pg16a) guard leg 340 passed, 867 checks, 0 fail cluster leg 325 passed, 898 checks, 0 fail `cluster_tests` 320 -> 325 by collection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

OffgridwithJD · 2026-09-14T15:36:30Z

The open wording question is settled: the message stays as it is. Recording the reason here rather than leaving it in a side channel, because "why is this one phrased differently" is exactly the question a future reader will ask.

@jdatcmd's argument, and it is not stylistic:

the existing three   errmsg("columnar: userinfo in \"%s\" is not supported", url)
this one             errmsg("columnar: userinfo in object-store endpoint \"%s\" ...", ep)

The three name the URL the CALLER wrote. This one names the endpoint the OPERATOR configured. A caller who wrote a perfectly clean s3://bucket/key and got back userinfo in "s3://bucket/key" is not supported would be looking at a string with no userinfo in it, being told it has userinfo. Matching the wording exactly would make the message identical and the referent wrong, which is the worse of the two.

The version here also agrees with its own hint — the message names the endpoint, the hint says remove it from the endpoint — so message and hint point at the same object. That is the property worth preserving; uniform phrasing across a different subject is not.

If exact uniformity across all four is wanted, the right move is to make the subject explicit in all of them — userinfo in URL "%s" / userinfo in endpoint "%s" — rather than flattening this one into the caller-URL form. That is a separate change touching three existing messages and their arms, and I have not made it here.

CI is 14/14, 0 pending, 0 failures on 1850be81.

jdatcmd

Approving at 1850be81, 14/14. Being explicit about which parts I verified myself and which I am relying on you for, since I did not rebuild — the box is mid-measurement for #890.

Verified directly from the source

The placement is real and checkable from the line numbers:

1435  if (strchr(ep, '@') != NULL)        <- the guard
1479  "requires a region option"          <- the demand that made it unreachable

Guard above demand, which is what lets it fire with no region configured. That is the whole finding, and it is visible without running anything.

The arms cover the direction that matters. test_an_at_sign_in_the_object_key_is_not_userinfo is the one I would keep hardest — @ is legal in an S3 key, and without that arm every other arm here would pass equally well against a guard that refused any @ anywhere. That is "make it say the other thing" applied properly rather than described.

test_the_guard_fires_without_a_region_configured is the placement claim stated as a test instead of trusted to a comment. A comment saying "this must go above the region demand" rots; an arm that fails when it moves does not.

Relying on you for

The removal proof (26 -> 5 -> 26 and 5 -> 3 -> 5, each asserting the .so mtime moved first) and the two suites legs. I have not rebuilt, so those are your measurements, not mine. Flagging it rather than implying I re-ran them.

On the wording

Answered in the channel and you have already put it on the PR, which is the right place — "why is this one phrased differently" is what a future reader asks, and the answer belongs where they will look.

The traps

PGC_SKIP_BUILD=0 skipping the build is a genuinely nasty one: the value that reads as "do not skip" is the value that skips, and the suite announced it on line 5 of its own output. What caught it was the .so timestamps, not the announcement — a printed fact nobody reads is not a guard.

And git checkout -- FILE reverting an uncommitted fix, so the control fails identically to the mutant: that one produces a removal proof where both arms are red for the same reason, which reads as a working proof if you only check that the mutant reddened. Your restore md5 assertion is what made it visible.

@jdatcmd

…he section commandprompt#1064 shipped unnumbered commandprompt#1064 ADDED A NEW INSTANCE OF THE DEFECT THIS BRANCH CATCHES, which is why the merge cannot be resolved by taking a side: either side leaves `test_objstore_endpoint_userinfo.py` as an unnumbered `###`, and the arm this branch adds reddens on main the moment it lands. main:3879 ### `test_objstore_endpoint_userinfo.py` -- userinfo in an object-store endpoint (commandprompt#995) Not in the numbering, not in the contents, still NAMED so the coverage arm passes. The same shape `test_iceberg_fdw.py` arrived in during commandprompt#1057, in the very next PR, written by the person who had just measured it. Found by @jdatcmd reviewing this branch against the merged tree rather than against either side of it. RESOLUTION, and the guard is the arbiter for every part of it: - the conflicted hunk keeps this branch's `## 37. test_iceberg_fdw.py` - main's two `###` headings go; commandprompt#1064's objstore BODY is kept unchanged - objstore becomes `## 38.` where it already sits, and `test_hilbert_cluster.py` moves to 39 -- body order and numbering must agree, because the commandprompt#1023 arm checks inversions as well as gaps - no `(commandprompt#995)` in the heading: none of the other 38 carries an issue reference - CHANGELOG keeps both entries (commandprompt#996 again) ANCHOR RULE, worth stating because @jdatcmd's first attempt at this resolution hit it: lowercase, drop anything outside `[a-z0-9 _-]`, spaces to hyphens. The UNDERSCORES STAY. An anchor that strips them (`commandprompt#38-testobjstoreendpointuserinfopy`) reddens `test_every_in_document_link_in_this_directory_reaches_a_heading` on its own, which is how they caught it. Removal proof on the MERGED tree, restoring from a COPY rather than from git, because this work is uncommitted and `git checkout --` restores to HEAD: CONTROL (resolved merge) 37 passed M objstore back to its ARRIVAL shape 1 failed <- the new arm, alone TESTS.md restored to 149db63b THE ARRIVAL SHAPE IS THE POINT, and it answers @jdatcmd's note that they could not reproduce a single failure by demoting a section. Demoting one that was already numbered leaves a gap and an orphaned contents entry, so all three arms redden. A section that ARRIVES unnumbered, with the contents consistent around it, is invisible to the other two and caught only by this arm. commandprompt#1057 and commandprompt#1064 were both the second kind, which is why neither was caught for a whole cycle. guard leg 341 passed, 870 checks, 0 fail guard_tests 341, cluster_tests 325, both re-derived by collection on the merged tree Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

@jdatcmd

… not `test_every_file_and_test_is_named_in_the_document` matches with `t not in text` over the WHOLE of TESTS.md, so a row naming a real test satisfies it wherever that row sits. The reverse arm passes for the same reason: a misplaced row names a test that exists, somewhere. BETWEEN THEM THEY ACCEPT ANY PERMUTATION OF EVERY TABLE IN THE FILE. Presence is not placement. WHAT WAS ACTUALLY WRONG, measured before the guard was written: ## 15 eight rows the classifier file defines, left behind when ## 16 was split out of it -- interleaved among section 15's own rows, not appended ## 44 no table; its row sat in ## 47 ## 46 no table; its two rows AND its `phs_nallocated` paragraph sat in ## 48 ## 47 its row sat in ## 48 ## 48 its row and its `serial_run / parallel_run` paragraph sat in ## 47 44 through 48 are ONE FOUR-SECTION SHIFT: each section's table and its load-bearing paragraph had come to rest under the next heading. Nothing here is a judgement about anyone's prose -- each paragraph names its own subject, so its home is determined. Section 15's eight are removed rather than merged, because section 16 already carries all eight with the descriptions its author wrote for that file. THE RULE IS KEYED ON THE ROW, NEVER ON THE HEADING, and that was learned the expensive way. A first draft matched `### Every test` and I published "4 tables, 0 failing on main" as its false-positive budget, in a blocking review and again in an approval. This document spells that heading at least four ways -- `### Every test` (4), `### Every arm` (24), `### The arms`, and ninety-odd sections headed with a backticked test name -- so the draft measured 4 of 58 sections and called it the population: heading-keyed draft 4 sections 0 failing (wrong population) row-keyed 58 sections 3 failing Corrected in public on commandprompt#1166 before anything was built on it. A count is a claim about its glob, and the corrected number made the case stronger: five occurrences, three standing on main, and nothing has ever checked it. ONE DIRECTION ONLY. A row may not name a foreign test. A test having no row is a different property and a larger change; `test_harness_deps_classifier.py` is short one row today (`test_a_file_that_only_PARSES_a_driver_import_is_job_runnable`) and that is recorded rather than smuggled in. NINE OCCURRENCES OVER 237 COMMITS, AND THE MECHANISM IS THE MERGE. The rule was run over every commit that touches this file. Six of the ten that introduced an occurrence are two-parent merges: this document is append-heavy, two branches each add a section, git merges the text with no conflict, and a table lands under the wrong heading. e62dd8d 09-09 s3 repaired b10e3f8 09-10 s15 repaired 6cdea5b 09-10 s15 repaired ad16800 09-10 s15 MERGE, NEVER REPAIRED -- still wrong 179 commits later 22ecc34 09-14 s36 repaired d1ff6e4 09-13 s36 MERGE "Merge main (commandprompt#1057) into commandprompt#1048" 5933b21 09-14 s38 MERGE "Merge main (commandprompt#1064) into commandprompt#1024" ce090c6 09-17 s46,47 MERGE 'author/main' into review/103... f74958e 09-17 s47,48 MERGE 'author/main' into review/106... 7217cfc 09-19 s72,72 (commandprompt#1164's rebase) WHAT THE EXISTING ARMS CATCH, measured by running them at each commit rather than by reasoning about them. A permutation that SPLITS a section duplicates a number, and `test_the_contents_list_is_numbered_in_order` and the link arm reject it at once -- they did, on 4c7ae02 and 0eb1f53. A permutation with the numbering intact is invisible: at ce090c6 AND f74958e, the two merges that produced the 44-48 rotation, both arms were fully green. AND THE OBVIOUS HAND REPAIR OF THE FIRST PRODUCES THE SECOND. On ee11772 the split section was renumbered and moved to the end, which satisfied both arms and carried the previous section's table with it. Red became green and the transposition survived to the merge. @jdatcmd found that, and it replaces a claim of mine that was too strong -- I had written "nothing has ever checked it", which is wrong in the one case where something did. A green numbering arm after a conflict repair is not evidence the document is right. A `##` HEADING THAT IS NOT A FILE SECTION ENDS THE SECTION TOO. Without that, a row under ordinary prose was charged to the file section above it -- a false positive on correct writing, and one that names a section the row is not even in, which is the failure mode that gets a guard switched off. Not live (five non-file `##` headings sit after a file section, no table rows under any of them) and reachable: two of the five are "What this corpus does NOT yet refuse" and "Traps this corpus records", exactly where somebody writes a table of test names. @jdatcmd planted it. Driven both ways, and the fix costs nothing on the real document -- 13 rows reported on main with it and without it. RED BEFORE GREEN, WITH THE SHIPPED ARM AND NOT A SCRIPT THAT RESEMBLES IT. This was built repair-first, so the red step was owed rather than taken, and it was taken before pushing: `main`'s TESTS.md restored under the new arm, the arm unchanged. 101 pass + 2 fail every table row names a test the section's own file defines: got 'test_harness_deps.py: test_the_classifier_catches_a_module_scope_driver_import belongs to test_harness_deps_classifier.py ... 7 more classifier rows ... test_index_fetch_penalty_crossover.py: test_native_chunk_length_bound belongs to test_native_chunk_length_bound.py test_index_fetch_penalty_crossover.py: test_parallel_scan_cost belongs to test_parallel_scan_cost.py test_parallel_scan_cost.py: test_a_parallel_index_build_covers_the_whole_table belongs to test_parallel_am_scan.py test_parallel_scan_cost.py: test_index_fetch_penalty_crossover belongs to test_index_fetch_penalty_crossover.py test_parallel_scan_cost.py: test_parallel_am_scan belongs to test_parallel_am_scan.py' want 'none' Thirteen rows across three sections, which is what the independent sweep counted before the arm existed -- TWO IMPLEMENTATIONS AGREEING, rather than one instrument checked against itself. The second failure is `test_every_file_and_test_is_named_in_the_document` and it is the expected one: `main`'s document does not yet name this branch's two arms. Restoring the repaired document returns 103 pass + 0 fail, byte-identical (0a0034d7f3f6). ALSO PROVED BY REMOVAL AGAINST THE REPAIRED DOCUMENT, so the arm is not merely reading a state it was written around. One row moved into the previous section, restored byte-identical (7f720954f8c6 both ways): got 'test_index_fetch_penalty_crossover.py: test_parallel_scan_cost belongs to test_parallel_scan_cost.py' want 'none' The fixture arms carry the control beside the offender and the case that decided the rule's shape: a table carried by NO HEADING AT ALL, which is how a quarter of this document is written and what a heading-keyed sweep scores clean either way. guard leg 384 passed, want 384, on pg15a/16a/17a/18a/19a 1064 checks, 0 failed, on each cluster leg 462, re-derived in the same run and unmoved, as expected 1537 pass + 0 fail + 1 unrun (ICU collation, absent here) docs_style.sh PASSED, and it reads CHANGELOG.md 382 -> 384 derived by collection, never by adding two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ZB6zgwuzg97GHoZaMUnPu

jdatcmd approved these changes Sep 14, 2026

View reviewed changes

jdatcmd merged commit 16bb2e7 into commandprompt:main Sep 14, 2026
14 checks passed

jdatcmd mentioned this pull request Sep 14, 2026

test: a test file documented outside the numbering is invisible (#1024) #1066

Merged

linuxhikerpm mentioned this pull request Sep 14, 2026

test: the piped-loop sweep needs a population, not just a detector (#1033) #1062

Merged

Navigation Menu

fix: refuse userinfo in an object-store endpoint (#995) - #1064

fix: refuse userinfo in an object-store endpoint (#995)#1064
jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:fix-995-endpoint-userinfo

OffgridwithJD commented Sep 14, 2026

Uh oh!

OffgridwithJD commented Sep 14, 2026

Uh oh!

jdatcmd left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

OffgridwithJD commented Sep 14, 2026

Two shapes, and only one was ever caught

The second shape is why this is a guard and not a message change

Placement: before the scheme and region demands

Removal proof, on both harnesses

Two traps walked into while measuring, both already in the record

Tests

One thing for @jdatcmd

Uh oh!

OffgridwithJD commented Sep 14, 2026

Uh oh!

jdatcmd left a comment

Choose a reason for hiding this comment

Verified directly from the source

Relying on you for

On the wording

The traps

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants